Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(bidi): pointerMove action needs to floor fractional values for x and y position #34191

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

whimboo
Copy link
Collaborator

@whimboo whimboo commented Jan 2, 2025

For elements that are only 1 pixel in size, rounding can cause the click to occur outside the element’s rect.

For example, a 1px div at (8,8) would not be reached if the click is rounded to (9,9).

This will fix the test page/page-click.spec.ts :: should click the aligned 1x1 div.

@yury-s can you please review? Thanks.

This comment has been minimized.

This comment has been minimized.

@dgozman
Copy link
Contributor

dgozman commented Feb 4, 2025

@whimboo Do you plan to update this PR or should we close it?

@whimboo
Copy link
Collaborator Author

whimboo commented Feb 4, 2025

@whimboo Do you plan to update this PR or should we close it?

Ideally yes, but I'm lacking time at the moment. Could someone from your side assist and finish this patch? I would appreciate.

…x and y position.

For elements that are only 1 pixel in size, rounding can
cause the click to occur outside the element’s rect.

For example, a 1px div at (8,8) would not be reached if
the click is rounded to (9,9).
@yury-s yury-s force-pushed the bidi-perform-action-move-fractional branch from 8af1764 to c062552 Compare February 7, 2025 00:15
@yury-s
Copy link
Member

yury-s commented Feb 7, 2025

I've pushed rebased version to whimboo:bidi-perform-action-move-fractional

@yury-s yury-s merged commit 6fddefd into microsoft:main Feb 7, 2025
29 of 31 checks passed
@yury-s
Copy link
Member

yury-s commented Feb 7, 2025

On a closer look, the problem is specific to Firefox implementation of Bidi as Chromium does not throw on fractional coordinates and handles them just fine (as underlying CDP implementation does, I guess). I see that in the spec the coordinates are typed as js-int. A better approach would be to fix the spec to allow passing precise fractional coordinates and do rounding on Firefox end if needed. @whimboo wdyt?

Copy link
Contributor

github-actions bot commented Feb 7, 2025

Test results for "tests 1"

11 flaky ⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [chromium-library] › tests/library/popup.spec.ts:264:3 › should not throw when click closes popup @ubuntu-20.04-chromium-tip-of-tree
⚠️ [webkit-library] › tests/library/proxy.spec.ts:63:3 › should work with IP:PORT notion @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:93:11 › should proxy local network requests › by default › localhost @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:178:3 › should exclude patterns @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/tracing.spec.ts:571:5 › should ignore iframes in head @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-leaks.spec.ts:161:5 › waitFor should not leak @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-screenshot.spec.ts:345:5 › page screenshot › should work while navigating @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:205:3 › should upload multiple large files @webkit-ubuntu-22.04-node18

37801 passed, 655 skipped
✔️✔️✔️

Merge workflow run.

@whimboo whimboo deleted the bidi-perform-action-move-fractional branch February 10, 2025 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants